Indentation inconsistent throughout this file. Might be worth running dotnet format on it.

Tried to do this now, looks like it doesn't work the same with strings within code unless I missed something. I could try to fix it manually too, just not sure if there's a better way

I don't like us having to do this manually. Did we look into source generation as an alternative? https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/reflection-vs-source-generation?pivots=dotnet-8-0#reflection

That's fair, it does require a lot more testing and is more code to maintain. One reason to continue with this approach is it follows the pattern we established in our other .NET repos of always using Utf8JsonReader for reading Json, which is more efficient compared to the JsonSerializer.Deserialize method. Also, we already added it to the main branch without variants/allocation/telemetry and this PR is re-adding it for merge conflict purposes.

I'm not sure, but from what I can tell here it looks like source generation isn't available in all of the versions we support, since it's only for C# 9.0 and later, and .NET 6.0 and later.

Should we handle the case that one of variant and users is missing? For example,

"allocation": {
    "user": [
        {
            "users": ["a","b"]
        }
    ]
}

This user allocation will not take effect. Do we still need to put it in the configuration or throw exception?

Good question, I think it should check to see if they aren't empty, so if there's anything inside of "user", either a list of users or a variant name, then it would be added. I thought since we shouldn't necessarily be doing any feature management logic, it would make sense to just exclude it if it's completely empty, but otherwise add the object. So in this case that you put up, I think it should be added. I don't think we need to throw an exception though, since we never did something like that if conditions was empty, for example.

Agree with Amer here. Feature Management library is responsible for checking the validity of a feature flag. Provider library just reads and flattens whatever is stored in AppConfig.

Min version could be a constant variable.

I can't actually make it a constant because the value is new Version(), but I could just put FeatureManagementMinimumVersion as a readonly class variable?

I was imagining a const string "3.2.0" which is local to this method. And we use Version.Parse to compare versions.

From and To are required fields. Why change to nullable?

Oh good point, I was thinking to do that so we could tell if they were set later on in the code when the percentile object is added to the key values output, but I don't think there's a point in doing that anyway. They should just be 0 by default like it was before. Regardless it should be wrong to be nullable if they're required

This interface is accumulating too many methods that apply only to a particular implementation. I'm wondering if we should repurpose the InvalidateCache method instead of introducing a new one. We could rename it to something like ResetInternalState(ConfigurationSetting setting = null) so that it fits both use cases. In all situations, we call this method before processing adapters.

cc @jimmyca15

I think you're right that at least something should change here - having the new method seems weird since it's just for this one case. I think a rename works based on where we're calling InvalidateCache now and where the state needs to reset for the FeatureManagementKeyValueAdapter anyway.

I was imagining a const string "3.2.0" which is local to this method. And we use Version.Parse to compare versions.

How about OnChangeDetected and OnConfigUpdated?

Sure, I think that's clearer for the first one